Skip to content

Fix ECDSA DER edge cases - #20

Merged
thieman merged 1 commit into
mainfrom
thieman/fix-ecdsa-der-edge-cases
Jun 25, 2026
Merged

Fix ECDSA DER edge cases#20
thieman merged 1 commit into
mainfrom
thieman/fix-ecdsa-der-edge-cases

Conversation

@thieman

@thieman thieman commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

What this PR changes

Closes #15.

This PR fixes two edge cases in the helper that converts ECDSA signatures from CNG's P1363 format into DER format.

The helper is p1363_to_der() in src/signer/ec.rs.

It now:

  1. encodes DER sequence lengths correctly when the content length is exactly 128 bytes;
  2. safely handles all-zero r or s components without indexing past the end of a slice.

Background: why are there two ECDSA formats?

ECDSA signatures are made of two numbers, traditionally called r and s.

Windows CNG returns ECDSA signatures in a raw / P1363-style format:

r || s

That means the two numbers are simply concatenated together, each using the fixed size for the curve.

TLS and X.509 tooling generally expect ECDSA signatures as ASN.1 DER:

SEQUENCE {
  INTEGER r,
  INTEGER s
}

TLS 1.3 describes ECDSA signatures as DER-encoded ECDSA-Sig-Value structures; see RFC 8446 section 4.2.3 and the related verified erratum text: https://datatracker.ietf.org/doc/html/rfc8446#section-4.2.3, https://errata.rfc-editor.org/eid5868/

DER is defined by ITU-T X.690: https://www.itu.int/rec/T-REC-X.690

So this crate has to convert CNG's raw r || s bytes into the DER form rustls peers expect on the wire.

What was wrong

1. Length 128 was encoded incorrectly

DER has two ways to encode lengths:

  • short form for lengths 0 through 127;
  • long form for lengths 128 and above.

The old code used short form for length <= 0x80. But 0x80 is 128, so it must use long form. In BER, byte 0x80 has a special "indefinite length" meaning; DER forbids that form.

That means an ECDSA signature whose DER sequence content happened to be exactly 128 bytes could be encoded as:

30 80 ...

instead of the valid DER form:

30 81 80 ...

This is rare, but possible with P-521-sized signatures. When it happens, strict DER parsers can reject the signature and the TLS handshake can fail.

2. All-zero components could panic

ECDSA signatures should not have r = 0 or s = 0. CNG should not produce such signatures for valid keys.

Still, the helper had loops like "while the first byte is zero, remove it". If every byte was zero, the slice could become empty and the next r[0] / s[0] access would panic.

That is not expected to be attacker-reachable through normal CNG signing, but it is an easy robustness fix in the same function.

Why this fix is the right thing to do

The DER length fix follows X.690's short-form vs long-form boundary: short form is only for values less than 128.

The all-zero fix preserves a single zero byte. That is the canonical DER INTEGER encoding for the integer value zero:

02 01 00

Normal signatures are unchanged. We still strip unnecessary leading zeroes from positive integers, and we still add a sign-padding zero when the high bit would otherwise make the DER INTEGER look negative.

How to read this if you are not a crypto expert

This is not changing the math of ECDSA. It is changing the envelope used to serialize the signature.

Think of it like writing a length-prefixed message. The old code used the wrong length-prefix form at exactly one boundary value. Most signatures never hit that exact size, but if one does, another implementation may say "this message is not valid DER" and abort.

Validation

This PR adds focused unit tests for both edge cases:

  • a constructed signature whose DER sequence content length is exactly 128 bytes must start with 30 81 80;
  • an all-zero raw signature must encode as a sequence containing two INTEGER zero values and must not panic.

The change surface is limited to src/signer/ec.rs.

@thieman
thieman marked this pull request as ready for review June 25, 2026 14:36
@thieman
thieman requested a review from a team as a code owner June 25, 2026 14:36

@webern webern left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, description is helpful. Not a crypto expert.

@thieman
thieman merged commit 216d1e0 into main Jun 25, 2026
10 checks passed
@thieman
thieman deleted the thieman/fix-ecdsa-der-edge-cases branch June 25, 2026 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix ECDSA P1363-to-DER edge cases

2 participants